Skip to content

Added ManagementConsole API and some missing tests. #203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Dec 10, 2014
Merged

Added ManagementConsole API and some missing tests. #203

merged 11 commits into from
Dec 10, 2014

Conversation

guillermoandrae
Copy link
Contributor

No description provided.

* @param array $hash
* @return \Guzzle\Http\EntityBodyInterface|mixed|string
*/
protected function get($uri, $hash)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overwriting the parent method with a different signature is a bad idea IMO. You should use a different method name instead

@guillermoandrae
Copy link
Contributor Author

@stof thanks for the speedy and thorough review.

$api = $this->getApiMock();
$api->expects($this->once())
->method('get')
->with('/setup/api/configcheck')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should check that get is called with the hash in the headers too, not only with the second argument

@guillermoandrae
Copy link
Contributor Author

@stof I went ahead and added the remaining Enterprise API tests.

@guillermoandrae
Copy link
Contributor Author

@stof anything else I need to add here?

@guillermoandrae
Copy link
Contributor Author

Any word on this?

@stof
Copy link
Contributor

stof commented Nov 17, 2014

@guillermoandrae I'm not a maintainer on this repo. So it is not me merging this (I just provide review on the repo to help the maintainers)

@guillermoandrae
Copy link
Contributor Author

Got it, @stof. Thanks very much for the feedback!

Maybe @pilot or @cursedcoder ... can one of you guys take a look at this?

@pilot
Copy link
Contributor

pilot commented Dec 10, 2014

@guillermoandrae 👍

pilot added a commit that referenced this pull request Dec 10, 2014
Added ManagementConsole API and some missing tests.
@pilot pilot merged commit 40207d4 into KnpLabs:master Dec 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants